Avoid circular dependencies & add dependency-cruiser #475
Conversation
There was a problem hiding this comment.
I'm all for optimizing our bundle and this seems like an awesome tool. I still have questions about the dont-import-from-index thing though. Not doing that creates quite a bit of repetitiveness and I want to make sure we're going down the right path.
What is the reason we can't import from index but Web and other sewing-kit apps are recommended to do so?
Also if we are going to switch over to specific imports only I think we should probably create a custom linting rule that explains why especially since this is the preferred way of importing in some of our other codebases.
BPScott
changed the title
My original plan was to add depcruiser as part of the linting phase, as while we use eslint-plugin-import (via eslint-plugin-shopify) and it has a no cylical imports rule it was never triggered even though we have it enabled. Further investigation suggests that it requires some additional configuration to work with typescript. I'll look into that and try and get it merged into eslint-plugin-shopify: |
|
Yay I've found out that eslint should have already been complaining about circular dependencies, but it wasn't correctly configured for TypeScript files I'll stick that into this PR and favour it over depcruiser to identify circular imports - no point in adding a new tool when an existing one can suffice.
Forcing me to think this through and offer good reasoning is super-appreciated. Thank you for doing so :)
I think this isn't a "us vs web" thing but a "accessing sibling components vs accessing components that exist anywhere else in the hierarchy" thing. It seems "us vs web" because the majority use-case in Polaris is a shared component importing one of its sibling shared components (as we have a single components folder) while this is just one of several ways that one component can import another within web. The existing rule of "import from the component index" only applies if the file you're importing is NOT a sibling of the current file. In web most of the time the shared component you're importing lives in a different folder to the current file. I think at the moment your concern that this PR would violate the web-foundations standards is because you're trying to apply the rule in all cases rather than noting this exception to the rule. As that is a new rule, there is bound to be some existing violations as it is not currently being enforced in Web but they should be refactored. To highlight the varying ways that you can import in Web, here's a list of places a component can live in Web ordered from on a scale of most to least specific :
There's a a couple of ways you can import here:
As you can see the way component imports work in polaris (i.e you're importing a sibling) only crops up some of the time in web. |
🎉
I'm not as concerned with that we would have to follow the conventions Web is setting. If we decide our ideal convention of importing is different from Web that's something I'm good with I just think its probably a good idea to establish a convention. My question more was "assuming that importing from the index file is the ideal way of working when importing from a different folder I still don't quite understand why we would have technical limitations that would prevent us from doing so but Web doesn't?"
Totally agree with the rule. I feel though as if the core of the argument here is that we might not completly agree on where we wanna go with this. My main point is that I think we should have a convention around imports and I usually feel like the conventions Web is setting are a good place to start. To lay out the different parts of how those work in my experience: Components, utilities etc are always imported from an index file: // global component
import {FeedbackModal} from 'components';
// local component
import {Action} from './components';The added rule seems to make sure that a path always leads to an individual file or folder. This for example happens in tests for a component where you would import the component from its actual file and not the index. // bad
import FeedbackModal from '..';
// good
import FeedbackModal from '../FeedbackModal';In conclusion I think its important we first establish what our ideal way of importing is going to be and then what we need to do to get there. I have assumed so far that we wanna do it the same way Web does it and the only thing I see standing in the way of that right now is the technical limitations around the index imports and awareness but I feel like you might have some additional ideas regarding how this should all work and we should probably align on them. Having that said having linting help us make sure we don't create circular dependencies sounds great 💯 |
|
Yo @kvendrik, this has now been rebased and is ready for review. I have not yet enabled the Running Conveniently I think these might be solved by your AppProvider refactor. |
|
Info from the commit message about what's going on here: Importing from the component index leads to circular depdendencies that This commit adds tooling to help idenify cycles and stop them, so that
Fix current ancestort imports to import from direct files. This is not |
Importing from the component index leads to circular depdendencies that cause issues when trying to optimize builds upstream. See this essay https://github.com/Shopify/sewing-kit/blob/2bbf40fee4b6fc6e8a2cbbab1b743b47532b98eb/src/tools/webpack/tests/config/rules/javascript.test.ts#L215-L268 This commit adds tooling to help idenify cycles and stop them, so that sewing-kit can remove its workarounds. * Depdendency Cruiser (`yarn run depcruise-validate`) shall help idenfity cycles. * shopify/no-ancestor-directory-import is an eslint rule that bars imports from parent indexes, which is the most common cause of cycles. Fix current ancestort imports to import from direct files. This is not enforced in tests because. test files will not be imported by anything else so there is no danger of cylical imports.
WHY are these changes introduced?
In another issue @GoodForOneFare pointed out that having circular dependencies causes pain downstream.
If we were to remove circular dependencies then sewing-kit should be able optimize polaris-react more effectively.
This basically means changing how we import components - we should reference individual components specifically instead of importing from the components index. For example, assuming the following code exists in
src/components/MyComponent/MyComponent.tsx:Bad, as it results in circular dependencies:
Good, no circular dependencies:
WHAT is this pull request doing?
Updates most existing cases where we import from the components index to import from specific components instead.
Adds https://github.com/sverweij/dependency-cruiser as a tool to audit these dependencies.
Run
yarn run depcruise src --validateto see a report.I have not yet added the above to run as part of CI as there is still some complexity that needs tiding that isn't as simple as renaming imports.
How to 🎩
yarn run depcruise src --validateto check existing errors